Correctly implement fuzzers for new encryption streams - #131306
Correctly implement fuzzers for new encryption streams#131306alinpahontu2912 wants to merge 7 commits into
Conversation
|
Tagging subscribers to this area: @dotnet/area-meta |
There was a problem hiding this comment.
Pull request overview
This PR updates the fuzzers for the new ZIP encryption streams to exercise the actual ZipArchive encryption/decryption paths (ZipCrypto and WinZip AES) rather than constructing the internal crypto streams via reflection.
Changes:
- Replace reflection-based stream construction with
ZipArchive.CreateEntry(..., password, ZipEncryptionMethod)+Open/OpenAsync(password)round-trip workflows. - Add validation that the entry is encrypted and the expected encryption method is recorded, and verify plaintext round-trips correctly (sync + async).
- Add a “wrong password” scenario intended to ensure failures are handled as
InvalidDataException(but it currently doesn’t assert failure if no exception is thrown).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/ZipCryptoStreamFuzzer.cs | Moves from reflected ZipCryptoStream.Create to ZipArchive-based encrypt/decrypt round-trip for ZipCrypto (sync + async). |
| src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/WinZipAesStreamFuzzer.cs | Moves from reflected WinZipAesStream.Create to ZipArchive-based encrypt/decrypt round-trip for AES128/192/256 (sync + async). |
Comments suppressed due to low confidence (2)
src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/ZipCryptoStreamFuzzer.cs:101
- The “wrong password must fail” check currently passes even if decryption succeeds without throwing. Use Assert.Throws so the fuzzer reliably flags regressions where a wrong password is accepted.
// Decrypting with a wrong password must fail cleanly with InvalidDataException, never crash.
try
{
using Stream stream = readEntry.Open("wrong-password".AsSpan());
stream.CopyTo(Stream.Null);
}
catch (InvalidDataException)
{
// Expected: the header password verifier rejects the wrong key.
}
src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/WinZipAesStreamFuzzer.cs:113
- The “wrong password must fail” check currently passes even if decryption succeeds without throwing. Use Assert.Throws so the fuzzer reliably flags regressions where a wrong password is accepted.
// Decrypting with a wrong password must fail cleanly with InvalidDataException, never crash.
try
{
using Stream stream = readEntry.Open("wrong-password".AsSpan());
stream.CopyTo(Stream.Null);
}
catch (InvalidDataException)
{
// Expected: the AES password verifier / HMAC rejects the wrong key.
}
|
@MihuBot fuzz WinZipAesStreamFuzzer |
|
Ran the fuzzer(s) successfully. Code coverage reports: |
|
@MihuBot fuzz ZipCryptoStreamFuzzer |
|
Ran the fuzzer(s) successfully. Code coverage reports: |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@MihuBot fuzz ZipCryptoStreamFuzzer |
|
@MihuBot fuzz WinZipAesStreamFuzzer |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@MihuBot fuzz ZipCryptoStreamFuzzer |
|
Ran the fuzzer(s) successfully. Code coverage reports: |
MihaZupan
left a comment
There was a problem hiding this comment.
Thanks, this looks a lot better!
|
@MihuBot fuzz ZipEncryptionStreamFuzzer |
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Buffers; |
|
Ran the fuzzer(s) successfully. Code coverage reports: |
| RoundTrip(buffer.Memory, password, method, async: true).GetAwaiter().GetResult(); | ||
| } | ||
|
|
||
| private static async Task RoundTrip(ReadOnlyMemory<byte> content, string password, ZipEncryptionMethod method, bool async) |
There was a problem hiding this comment.
Just making the note that you may see less edge-case coverage because your decryption logic is only exercising well-formed inputs produced by our own library.
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/ZipEncryptionStreamFuzzer.cs:59
passwordBytescan have an odd length when the newline delimiter lands on an odd offset.MemoryMarshal.Cast<byte, char>(passwordBytes)then throws because the span length isn't a multiple ofsizeof(char), which will prematurely abort many fuzz cases and reduce coverage of the encryption streams.
int newlineIndex = payload.IndexOf((byte)'\n');
ReadOnlySpan<byte> passwordBytes = newlineIndex < 0 ? ReadOnlySpan<byte>.Empty : payload.Slice(0, newlineIndex);
ReadOnlySpan<byte> contentBytes = newlineIndex < 0 ? payload : payload.Slice(newlineIndex + 1);
src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/ZipEncryptionStreamFuzzer.cs:45
- The length check/comment imply only the method-selector byte is required, but the code also intentionally skips a second byte as padding to keep the remaining payload 2-byte aligned. Updating the comment avoids confusion about why
bytes.Length < 2is the cutoff and whySlice(2)is used.
// The first byte is consumed below as the encryption-method selector, so an empty
// input has no byte to interpret and there is nothing to fuzz.
if (bytes.Length < 2)
{
return;
Correctly implement fuzzers fro the new encryption streams: zipcryptostream and winzipaesstream